create-protocol-parameters-update: validate cost models' size by smelc · Pull Request #1012 · IntersectMBO/cardano-cli

smelc · 2025-01-09T15:48:09Z

Changelog

- description: |
    create-protocol-parameters-update: validate cost models' size
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Fixes #928

How to trust this PR

I've taken the rules for checking V2 from here: https://github.com/IntersectMBO/cardano-api/blob/4ab850fcdefe8536fb98274ba5eaf54030a852e8/cardano-api/internal/Cardano/Api/Genesis.hs#L245 (written by @carbolymer)
For PV1 and PV3, I looked up the Plutus implementation

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

cardano-cli/src/Cardano/CLI/Read.hs

+      Just (model :: L.CostModel) -> do
+        let actual = Map.size (L.costModelToMap model)
+            expected = languageToParameterCount lang
+        unless (expected == actual) $ throwE $ CostModelsErrorWrongSize expected actual
+        return ()


smelc · 2025-01-09T15:50:00Z

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Action.hs

+hprop_golden_conway_governance_action_create_protocol_parameters_bad_costmodel_size =
+  propertyOnce . H.moduleWorkspace "tmp" $ \tempDir -> do
+    -- This file has 184 entries, whereas PV1 requires 166
+    -- TODO @smelc this test does not pass: the command succeeds, whereas it should fail!


My guess is that for PV1 the ledger deserialization fixes the problem by filling in missing parameters. Investigating. (this could also be an issue if the ledger is automatically trimming extra parameters, I need to check that too).

Yes, it's something along those lines. The defaulting logic may also change depending whether the cost model is an array or a map. 🫠

ledger is automatically trimming extra parameters

I think it is not, we're only doing trimming in alonzo genesis reading, because the node didn't want to start in some cases.

carbolymer · 2025-01-10T08:41:26Z

cardano-cli/src/Cardano/CLI/Read.hs

+    L.PlutusV2 ->
+      let nbParamNames = length $ allValues @PlutusV2.ParamName in -- 185
+      caseShelleyToBabbageOrConwayEraOnwards
+        (const $ nbParamNames - 10) -- Ten parameters were added to V2 in Conway, need to remove them here


I'm not so sure about hardcoding that number 10. We should account for the fact that the number of params can change in the future, however it's unlikely. That's why there's jumping over many hoops in Cardano.Api.Genesis and Test.Cardano.Api.Genesis, so we will get a clear signal when things unexpectedly change.

carbolymer · 2025-01-10T08:44:44Z

cardano-cli/src/Cardano/CLI/Read.hs

  return costModels
+ where
+  checkCostModelSize :: L.CostModels -> L.Language -> ExceptT CostModelsError IO ()
+  checkCostModelSize models lang =


I think those checks should be in cardano-api so we can reuse them in decoding genesis as well, because genesis can contain PV2 and PV3 models. This is used only in starting testnets afaik.

carbolymer · 2025-01-10T08:46:05Z

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Action.hs

+hprop_golden_conway_governance_action_create_protocol_parameters_bad_costmodel_size =
+  propertyOnce . H.moduleWorkspace "tmp" $ \tempDir -> do
+    -- This file has 184 entries, whereas PV1 requires 166
+    -- TODO @smelc this test does not pass: the command succeeds, whereas it should fail!


Yes, it's something along those lines. The defaulting logic may also change depending whether the cost model is an array or a map. 🫠

ledger is automatically trimming extra parameters

I think it is not, we're only doing trimming in alonzo genesis reading, because the node didn't want to start in some cases.

carbolymer · 2025-01-10T08:48:32Z

cardano-cli/src/Cardano/CLI/Read.hs

+  languageToParameterCount = \case
+    L.PlutusV1 -> length $ allValues @PlutusV1.ParamName -- 166
+    L.PlutusV2 ->
+      let nbParamNames = length $ allValues @PlutusV2.ParamName in -- 185


Suggested change

let nbParamNames = length $ allValues @PlutusV2.ParamName in -- 185

let nParamNames = length $ allValues @PlutusV2.ParamName in -- 185

😄

github-actions · 2025-02-25T02:02:40Z